Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dvc config to store and access studio.token #3768

Merged
merged 4 commits into from
Apr 28, 2023
Merged

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Apr 27, 2023

1/6 main <- this <- #3701 <- #3771 <- #3777 <- #3778 <- #3779

One of the checkboxes from #3574

This PR switches us from using VS Code's secret storage API to the dvc config command for storing a user's studio.token.

Demo

Screen.Recording.2023-04-27.at.1.22.04.pm.mov

Note: We are not currently supporting users using config.local to store a studio.token value in monorepos.

@mattseddon mattseddon added the product PR that affects product label Apr 27, 2023
@mattseddon mattseddon self-assigned this Apr 27, 2023
extension/src/util/appdirs.tes.ts Outdated Show resolved Hide resolved
extension/src/util/appdirs.tes.ts Outdated Show resolved Hide resolved
@mattseddon mattseddon changed the title use dvc config to store and access studio.token Use dvc config to store and access studio.token Apr 27, 2023
@@ -71,6 +72,10 @@ export class DvcExecutor extends DvcCli {
return this.blockAndExecuteProcess(cwd, Command.COMMIT, ...args, Flag.FORCE)
}

public config(cwd: string, ...args: Args) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] Config doesn't really fit with the reader/executor model as it can both read and write the config. For now, I've put it into executor.

)
}

const cwd = this.dvcRoots[0]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] If there is only 1 dvc root in the workspace then we try to clear out the local and global configs. If the user has set the token into their --system config this won't work but I think this approach will get us close enough with the vast majority of users using the extension to manage their token.

@@ -579,6 +616,10 @@ export class Setup
return this.workspaceChanged.fire()
}

private getCliCompatible() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] Had to wrap this so I could stub it.

return
}

if (this.dvcRoots.length !== 1) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] same thing here we are not supporting --local wrt to multiple projects being in the workspace.

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

)
expect(getDVCAppDir()).toStrictEqual(mockedAppDir + '/' + 'dvc')
})
it('should return the correct path on Windows', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but don't we tent to add lines between it blocks?

await this.internalCommands.executeCommand(
AvailableCommands.CONFIG,
cwd,
Flag.GLOBAL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention somewhere that the Studio token is saved globally? Or that users with --system can't remove their tokens?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs say this:

image

I did consider trying to accommodate "all possibilities" but really we are trying to help beginners with these actions. If the user is competent enough to save a token in their --system config then I do not think they'll try to manage it in this way. I feel like even trying to delete --local is really going above and beyond + makes the code messy enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if anyone complains we will revisit

@mattseddon mattseddon enabled auto-merge (squash) April 28, 2023 20:28
@codeclimate
Copy link

codeclimate bot commented Apr 28, 2023

Code Climate has analyzed commit 0e2baf7 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 84.9% (85% is the threshold).

This pull request will bring the total coverage in the repository to 94.6% (-0.1% change).

View more on Code Climate.

@mattseddon mattseddon merged commit b92bd2b into main Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants